-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Don't allow 1d-arrays in plot_surface. #5166
Conversation
I have been wavering about whether or not this is a bug or a feature. I am leaning towards bug because when one has a 1D Z array, it can ambiguous which way the user intended it to be oriented. This check does still allow for broadcasting of inputs, so a Nx1 array would still work (and it would be a simple work-around for any users that depended on the atleast_2d() behavior). @tacaswell , I am assuming we are holding off on merging any non-critical PRs until after you branch 1.5? |
As questioned in the issue: Is there ANY useful usage of a 1-dim Z input? I can't imagine one. On other hand i spend time due to the current behavior hiding a bug in my code. |
Never claimed there was. The current behavior came about as the result of adding broadcasting abilities, which was specifically requested for 1-D X or Y inputs. The ability to broadcast Z was merely a natural side-effect of that feature. |
I should note that the documentation does say that inputs should be 2D. |
Sorry i am confused, i don't touch the broadcasting of x and y? |
I only mentioned broadcasting of X and Y because it was the addition of that feature a few years ago that introduced this "bug/feature" that you are fixing now. The only question is if it really is a bug or not. I am leaning towards it being a bug that needs to be fixed. |
Oh, it looks like the same problem is in |
Yes please. I plan to set up a 1.5.x and 2.x branches after tagging rc2 On Fri, Oct 2, 2015, 11:40 Benjamin Root notifications@github.com wrote:
|
@WeatherGod If you think this should be in 1.5 move it. |
This is non-critical. It can wait. |
FIX: Don't allow 1d-arrays in plot_surface.
@Tillsten or @WeatherGod can you also apply the same fix to I don't think think this breaks back compatibility because a) we can't quite figure out how it would work b) the new behavior matches the docstring. Inclined to not back-port this for 2.0.x, but it would be very easy to convince me otherwise. |
@tacaswell, this PR applied the fix to both plot_surface and plot_wireframe. As for backporting, I would be against that. The old code would have done something correct-ish if either the X or Y arrays were 2D. |
🐑 Sorry for missing that. RE backporting, sold. |
plot_wireframe requires 2D arrays. In recent versions of matplotlib passing a 1D array fails with an error: https://stackoverflow.com/questions/47225830/axes3d-plot-wireframex-y-z-error matplotlib/matplotlib#5166 matplotlib/matplotlib#3116
raise value error instead. fixes #3116.